Ref #1589: Add warning messages for migration annotation #3562

vitorsvieira · 2017-11-26T21:59:27Z

Regarding #1589, this PR addresses:

"{symbol} has an unparsable version number..." (RefCheck:694)
"{symbol} has changed semantics in version..." (RefCheck:698)

…er. RefCheck:694

…sion. RefCheck:698

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Added" instead of "Add")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

nicolasstucki · 2017-11-27T09:11:46Z

Please change the commit message to Ref #1593: Add warning messages for migration annotation

…d-changed-semantics-in-version # Conflicts: # compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java # compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

nicolasstucki · 2017-11-28T15:16:22Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+    val explanation =
+      hl"""$migrationMessage
+          |
+          |The symbol is marked with ${"@migration"} indicating it has changed semantics


You should not talk about symbols as this is a compiler internal concept, the user should not know about them. You should probably replace it with ${symbol.showLocated} or ${symbol.show}.

nicolasstucki · 2017-11-28T15:17:42Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+          |
+          |The symbol is marked with ${"@migration"} indicating it has changed semantics
+          |between versions and the ${"-Xmigration"} settings is used to warn about constructs
+          |whose behavior may have changed since version.""".stripMargin


"may have changed since version" -> "may have changed since version change"

nicolasstucki · 2017-11-28T15:17:50Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+    val explanation = {
+      hl"""The symbol is marked with ${"@migration"} indicating it has changed semantics
+          |between versions and the ${"-Xmigration"} settings is used to warn about constructs
+          |whose behavior may have changed since version.""".stripMargin


nicolasstucki · 2017-11-28T15:18:04Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+      hl"""${symbol.showLocated} has changed semantics in version $symbolVersion:
+          |$migrationVersion""".stripMargin
+    val explanation = {
+      hl"""The symbol is marked with ${"@migration"} indicating it has changed semantics


same (symbol)

vitorsvieira · 2017-11-28T20:07:57Z

I was always curious about this annotation. Is there any reason why it is not part of the standard library? ATM, it is available only internally private[scala] final class migration(message: String, changedIn: String) extends scala.annotation.StaticAnnotation.

Just out of curiosity, Swift provides the @available attribute, that IMO is an expanded
representation of scala's @migration. Maybe a combination of @deprecated and @migration

In Swift, @available(Platform+VersionNumber*, Args*) attribute can be used to indicate the whole declaration's lifecycle, relative to certain Swift language versions or certain platforms and operating system versions. Some of its arguments:

unavailable(version number)
introduced(version number)
deprecated(version number)
obsoleted(version number)
message(message)
renamed(new name)

In scala's ecosystem, this would mean the possibility to achieve something like:
@available(JVM1_8, ScalaJS1_0, ScalaNative1_0, introduced=2.4, deprecated=2.6, obsoleted=2.10, renamed="newMethodName")

Reference: https://developer.apple.com/library/content/documentation/Swift/Conceptual/Swift_Programming_Language/Attributes.html#//apple_ref/doc/uid/TP40014097-CH35-ID347

PS: Not a suggestion, just toughts ;)

allanrenucci · 2017-11-28T20:14:13Z

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

          false
      }
      if (changed)
-        ctx.warning(s"${sym.showLocated} has changed semantics in version $symVersion:\n${sym.migrationMessage.get}")
+        ctx.warning(SymbolChangedSemanticsInVersion(sym, symVersion, sym.migrationMessage.get))


While you're at it. Can you move this into the match. There is no need for this changed variable.
Also, symVersion is sym.migrationVersion.get so you don't need both of them

Sure. 1sec.

allanrenucci · 2017-11-28T20:37:10Z

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

-          ctx.settings.Xmigration.value < v
+    val xMigrationValue = ctx.settings.Xmigration.value
+    if (sym.hasAnnotation(defn.MigrationAnnot) && xMigrationValue != NoScalaVersion) {
+      sym.migrationVersion.get match {


Your match is not exhaustive

allanrenucci · 2017-11-28T20:38:37Z

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

+    if (sym.hasAnnotation(defn.MigrationAnnot) && xMigrationValue != NoScalaVersion) {
+      sym.migrationVersion.get match {
+        case scala.util.Success(symVersion) if xMigrationValue < symVersion =>
+          ctx.warning(SymbolChangedSemanticsInVersion(sym, symVersion, sym.migrationMessage.get))


You can remove sym.migrationMessage.get. It is the same as symVersion. I don't know why it was duplicated in the first place

I would have to change the final message if that is ok. Currently:

${symbol.showLocated} has changed semantics in version $symbolVersion: $migrationVersion

The migrationMessage provides either a version string or an error message from ScalaVersion.parse:

def failure = Failure(new NumberFormatException( s"There was a problem parsing ${versionString}. " + "Versions should be in the form major[.minor[.revision]] " + "where each part is a positive number, as in 2.10.1. " + "The minor and revision parts are optional." ))

It cannot be a failure since you're in the Success case of the match

allanrenucci · 2017-11-28T20:40:02Z

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

+    if (sym.hasAnnotation(defn.MigrationAnnot) && xMigrationValue != NoScalaVersion) {
+      sym.migrationVersion.get match {
+        case scala.util.Success(symVersion) if xMigrationValue < symVersion =>
+          ctx.warning(SymbolChangedSemanticsInVersion(sym, symVersion, sym.migrationMessage.get))


You're missing the pos

allanrenucci · 2017-11-28T20:40:37Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+          |
+          |The ${symbol.showLocated} is marked with ${"@migration"} indicating it has changed semantics
+          |between versions and the ${"-Xmigration"} settings is used to warn about constructs
+          |whose behavior may have changed since version change.""".stripMargin


You don't need to stripMargin

allanrenucci · 2017-11-28T20:40:45Z

compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

+    val explanation = {
+      hl"""The ${symbol.showLocated} is marked with ${"@migration"} indicating it has changed semantics
+          |between versions and the ${"-Xmigration"} settings is used to warn about constructs
+          |whose behavior may have changed since version change.""".stripMargin


You don't need to stripMargin

allanrenucci

You need to update your submodules:

git submodule update

allanrenucci · 2017-11-28T21:09:43Z

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

-          ctx.warning(s"${sym.showLocated} has an unparsable version number: ${ex.getMessage()}", pos)
-          false
+          ctx.warning(SymbolHasUnparsableVersionNumber(sym, ex.getMessage()), pos)
+        case _ => ()


Remove ()

…d-changed-semantics-in-version

vitorsvieira added 2 commits November 26, 2017 22:16

scala#1589: Add error message for symbol with unparsable version numb…

ff25154

…er. RefCheck:694

scala#1589: Add error message for symbol has changed semantics in ver…

121ab58

…sion. RefCheck:698

dottybot reviewed Nov 26, 2017

View reviewed changes

vitorsvieira added 2 commits November 26, 2017 23:08

scala#1589: Reuse migration message . RefCheck:694

7a511b3

scala#1589: Remove empty line.

ae876ef

vitorsvieira changed the title ~~#1593: Added error message for symbol has unparsable version and for symbol has changed semantics in version~~ #1593: Added warning messages for migration annotation Nov 26, 2017

vitorsvieira changed the title ~~#1593: Added warning messages for migration annotation~~ #1593: Add warning messages for migration annotation Nov 26, 2017

vitorsvieira changed the title ~~#1593: Add warning messages for migration annotation~~ Ref #1593: Add warning messages for migration annotation Nov 27, 2017

Merge branch 'master' into wip/1593-symbol-with-unparsable-version-an…

cbcd621

…d-changed-semantics-in-version # Conflicts: # compiler/src/dotty/tools/dotc/reporting/diagnostic/ErrorMessageID.java # compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

vitorsvieira changed the title ~~Ref #1593: Add warning messages for migration annotation~~ Ref #1589: Add warning messages for migration annotation Nov 27, 2017

nicolasstucki suggested changes Nov 28, 2017

View reviewed changes

Ref scala#1589: Updated error message explanation based on code review.

a7604e7

nicolasstucki approved these changes Nov 28, 2017

View reviewed changes

allanrenucci suggested changes Nov 28, 2017

View reviewed changes

vitorsvieira added 2 commits November 28, 2017 21:23

Ref scala#1589: Polishing.

33275a5

Ref scala#1589: Polishing.

6c11f68

allanrenucci suggested changes Nov 28, 2017

View reviewed changes

Ref scala#1589: Polishing.

e5c2d91

allanrenucci suggested changes Nov 28, 2017

View reviewed changes

vitorsvieira added 2 commits November 28, 2017 22:17

Ref scala#1589: Polishing.

3b8039e

Merge branch 'master' into wip/1593-symbol-with-unparsable-version-an…

435a5f9

…d-changed-semantics-in-version

allanrenucci approved these changes Nov 28, 2017

View reviewed changes

allanrenucci merged commit e98ee92 into scala:master Nov 28, 2017

vitorsvieira deleted the wip/1593-symbol-with-unparsable-version-and-changed-semantics-in-version branch November 29, 2017 10:27

Ref #1589: Add warning messages for migration annotation #3562

Ref #1589: Add warning messages for migration annotation #3562

Uh oh!

Conversation

vitorsvieira commented Nov 26, 2017

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

nicolasstucki commented Nov 27, 2017 • edited by allanrenucci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitorsvieira commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitorsvieira Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolasstucki commented Nov 27, 2017 •

edited by allanrenucci

Loading

vitorsvieira commented Nov 28, 2017 •

edited

Loading

vitorsvieira Nov 28, 2017 •

edited

Loading